WASIP3 Locking/Waiting Primitives#746
Conversation
…to sy/wasip3-locks
|
Needs a new release of |
|
Strange, these test failures don't repro for me locally on an M3 Mac |
|
Maybe it's the local clang version used to compile? I'm able to reproduce locally with stock |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
I was waiting for CI to be green before digging in much further here, but in retrospect there's no need as I don't think the code will change that much. I've left some comments below but overall this looks all good to me, thanks!
If you need help with CI just lemme know and I can try to help frob things.
| set(__wasip2__ ON) | ||
| elseif(WASI STREQUAL "p3") | ||
| set(__wasip3__ ON) | ||
| set(__wasi_cooperative_threads__ ON) |
There was a problem hiding this comment.
Could this be behind a #define/guard/etc for now while we're waiting for wasi-sdk/LLVM/etc to all get updated? That way we could go ahead and land all this in wasi-libc (untested) and follow-up with testing later. My gut is that it shouldn't be too hard to support building with/without coop threads within wasi-libc itself, but if that's too difficult to support then it's fine to wait for a build of LLVM with support for coop threds.
There was a problem hiding this comment.
HIgh-level question: how come this folder is changing? I would expect wasi-threads to be unaffected by wasip3 things, so I was a bit surprised to see this file change (and the one below)
|
|
||
| void __waitlist_wake_all(struct __waitlist_node **list) | ||
| { | ||
| struct __waitlist_node **prev = list; |
There was a problem hiding this comment.
Here prev is never modified so I think it could be replaced with direct usage of list?
| __unlockfile(f); | ||
| return c; | ||
| #else | ||
| if (a_cas(&f->lock, 0, MAYBE_WAITERS-1)) __lockfile(f); |
There was a problem hiding this comment.
I didn't expect musl to open-code locks in so many places...
| self->stdio_locks = f; | ||
| } | ||
|
|
||
| #ifdef __wasi_cooperative_threads__ |
There was a problem hiding this comment.
Could the #ifdef here go inside the function signature to avoid duplicating the function signature?
| int self_tid = __pthread_self()->tid; | ||
| if (f->lock.owner == self_tid) { | ||
| if (f->lockcount == LONG_MAX) | ||
| return -1; | ||
| f->lockcount++; | ||
| return 0; | ||
| } | ||
|
|
||
| // Try to acquire the lock | ||
| if (f->lock.owner != 0) | ||
| return -1; | ||
|
|
||
| f->lock.owner = self_tid; | ||
| f->lockcount = 1; |
There was a problem hiding this comment.
Could management/testing of .owner (or maybe a try_lock of some kind) go inside a header? Ideally the details of the lock implementation would be centalized in one *.h and one *.c is my rough thinking here
| // Allow recursive locking | ||
| if (f->lock.owner == __pthread_self()->tid) | ||
| return 0; | ||
|
|
There was a problem hiding this comment.
If this allows recursive locking, doesn't that mean that unlocking doesn't work? I'd expect some sort of counter to get increased for a recursive lock here or something similar to that.
There was a problem hiding this comment.
(or does musl already handle that case elsewhere via the return value of this function?)
If musl already handles this, a comment similar to below where I'd request that the testing of .owner be moved to some sort of function in the lock header file
| /* The implementation has no yield points, so locks can be no-ops. */ | ||
| static inline void lock(volatile int *lk) | ||
| { | ||
| } | ||
|
|
||
| static inline void unlock(volatile int *lk) | ||
| { | ||
| } |
There was a problem hiding this comment.
Could this compile down to some small debug-mode-only instrumentation perhaps?
There was a problem hiding this comment.
although this file may be unused by wasi-libc given that malloc lives elsewhere...
| #define FFINALLOCK(f) __lockfile((f)) | ||
| #define FLOCK(f) int __need_unlock = __lockfile((f)) | ||
| #define FUNLOCK(f) do { if (__need_unlock) __unlockfile((f)); } while (0) | ||
| #define __STDIO_LOCK_INIT {0, 0} |
There was a problem hiding this comment.
Aha looks like musl handles my concern below (above in review? who knows...). Not exactly how I'd imagine it being done but hey here it is
| #define FLOCK(f) int __need_unlock = __lockfile((f)) | ||
| #define FUNLOCK(f) do { if (__need_unlock) __unlockfile((f)); } while (0) | ||
| #define __STDIO_LOCK_INIT {0, 0} | ||
| #define __STDIO_LOCK_RESET(lock) do { (lock)->owner = 0; (lock)->waiters = NULL; } while (0) |
There was a problem hiding this comment.
This might be replacable with *lock = __STDIO_LOCK_INIT given what it's doing, but this is all buried in musl code so seems reasonable either way, happy to defer to you
2 participants
Includes #744
The first batch of changes for cooperative threading. This PR introduces the core locking and waiting primitives that will be used to implement many of the pthreads features, and reimplements the existing locks used throughout the codebase in terms of them.
sched_yieldin terms ofwasip3_thread_yield__wait.cconsisting of:struct __waitlist_node: a singly-linked list of TIDs__waitlist_wait_on: adds the current thread to the waitlist, allocating the node on the stack__waitlist_wake_(one,all): yields directly to one thread in the waitlist and unsuspends the rest__lock.cWEAK_(UN)LOCKmacro in addition to the existing(UN)LOCK, where the former is a no-op in co-operative threading buildsBecause these patches do not yet include the ability to spawn new threads, which will require llvm/llvm-project#175800, there is no additional testing added yet. Testing will be added as part of integration of the Open POSIX Test Suite, which will be coming in future PRs.